Apologies we have moved to using python 3.10 types since this PR is opened. So if you could drop Optional and instead use | None instead that would be great!

If you could also update your branch with current main then the formatting scripts and tests should catch this change for you

Correct me if I'm wrong but this doesn't sound quite right. The incoming document is usually a converted PDF file from a converter that hasn't yet been split. So this would mean the page_number probably doesn't exist yet in the meta data.

Either way the message "Processing page number: {current_page} out of {total_pages}" I think can be off for a few reasons. We are not just processing current_page we are processing all the pages right?

Also currently you don't offset total pages by current page so we could end up with message like "Processing page number: 10 out of 2" if page_number from meta was equal to 10 and this doc only had one page_break_character in it right?

I'd say let's drop this update and keep the leading newline character. We should utilize a DocumentCleaner after this splitter if we want to clean up this kind of leading and trailing whitespace type characters

I think this can produce an unwanted edge-case which is if keep_headers is True then they will be added to the content below on line 136 but we never reach there if the content is empty since we will hit the continue on 127. So currently it seems to me that this if self.keep_headers: doesn't do anything since once we reach here we will always skip this match anyways

never mind I see it's there already

I like the name of the test! But I don't see multiple headers. Could we update the example to use multiple headers and sub-headers?

I think ideally you could make a copy of the sample_text fixture that also includes page breaks.

This one is hard to defend – I think this is an artifact of me reworking and merging tests. I'll make sure to make this one consistent with its name, my bad!

I think we can remove this test now since we test the parent_headers in test_split_without_headers and test_basic_split

Let's not use these vague checks. Let's explicitly check that the expected doc has the expected header. So like

split_docs[X] == "Another Header"
split_docs[Y] == "H1"
...

Let's also run the same checks for this output

    for i in range(1, len(split_docs)):
        prev_doc = split_docs[i - 1]
        curr_doc = split_docs[i]
        if prev_doc.meta["header"] == curr_doc.meta["header"]:  # only check overlap within same header
            prev_words = prev_doc.content.split()
            curr_words = curr_doc.content.split()
            assert prev_words[-2:] == curr_words[:2]

Could we also add checks that the split_ids are as expected?

Let's also add split_id checks

Could we also update the assertions to be string comparisons instead of lengths? That would make it easier to see if something went wrong.

let's also add split_id checks here